# Engineering review

Oct 1, 2026 · `/plan-eng-review`, condensed

**Target:** `docs/build-plan.md`, `docs/demo-architecture.md` and `BACKLOG.md`, checked against the code built on 1 Oct (`hospital_zone/`, `platform_zone/`, `shared/`, `synthetic/`, `golden_path.py`, `calibrate.py`, `tests/`).

**How it ran.** The owner declined the interactive question flow, so every remedy below was a recommendation. On 1 Oct the owner ran sprint 1, which took the recommendations for A1, A2, A3, A5, C1, C3, C4 and C6; those are done. The rest stay **pending**. The sprint plan (`docs/sprint-plan.md`) schedules the recommended remedies and marks each one as needing confirmation at sprint 1 planning.

**Verdict.** The thin build is sound in shape: two zones, one export, labelled numbers, reproducible runs, calibration. Three things must change before any real patient data is loaded. The export is not authenticated. Small counts leak through differencing. Values for a site type with one site identify that site. A fourth problem is about schedule, not safety: the rule-based extractor only reads the synthetic passage format, so Epic 2 is larger than planned.

## Scope challenge

**What already exists.** The golden path runs end to end in about 2 seconds, and 71 tests pass. Reusable pieces:
- Statistics: numpy and scipy.
- Schema validation: jsonschema.
- Storage: SQLite, from the standard library.

Nothing here rebuilds a library that would do the job better, with one exception. Real document text extraction will need `python-docx` and `pdfplumber` (or `pypdf`). It should not be hand-written.

**Minimum change to reach the goal.** Keep the arrangement as built. The demo needs real-document reading, the privacy hardening below, a clinician review screen and a web framework. It does not need a job queue (finding A5) or PostgreSQL in the hospital zone (see A6).

**Complexity.** 30 Python modules (not counting package markers) across four packages, plus two runners, and no new services. That passes the 8-file threshold, so the complexity gate applies. The arrangement already exists and works, so it is recorded as **kept as built**. The owner did not answer the structure question.

**Search.** Web search is not available in this session, so this review uses in-distribution knowledge only. All recommendations are standard practice [Layer 1]: signed exports, allow-list de-identification, secondary cell suppression and Unicode normalisation.

**TODOs.** There is no `TODOS.md`. `BACKLOG.md` and `docs/sprint-plan.md` take the work instead.

**Distribution.** trial-os is not a git repository and has no CI (finding C6).

## 1. Architecture

```
 HOSPITAL ZONE                          exchange/                  PLATFORM ZONE
 deidentify ─▶ ClinicalStore ─▶ estimate/replay ─▶ export.gate ─▶ exp-*.json ─▶ ProfileStore.import_export
                 (plain SQLite)              │        (hash only)      ▲             (re-computes the same hash)
                 key file beside DB          │                         │
                                             └── cumulative series ────┘  ◀── A2: differences reveal small counts
                                                 per-site-type values ──  ◀── A3: one provincial site = one site
```

| # | Severity | Confidence | Where | Finding | Recommended remedy | State |
|---|---|---|---|---|---|---|
| A1 | P1 | 9/10 | `hospital_zone/estimator/export.py:73`, `platform_zone/profile/store.py:123` | The export's integrity check is a hash the bundle carries about itself. Anyone who can touch the file can edit a value and re-compute `content_sha256`, and the platform will accept it. The approval record is outside the hash. | Sign the bundle and its approval record with a hospital-held Ed25519 key. The platform pins the public key and refuses unsigned or mismatched bundles. | **done in sprint 1** |
| A2 | P1 | 9/10 | `hospital_zone/estimator/estimate.py:283` (cumulative enrolment), `:262` (Kaplan–Meier at-risk) | Each cell is at least 10, but consecutive cells differ by less. On seed 7, **24 monthly enrolment counts below 10** can be recovered by subtraction, and the provincial at-risk series reveals 1–4 dropouts a month. This defeats the minimum-cell rule (hard rule 1). | Export cumulative enrolment by quarter, not month. The gate also checks that every difference between consecutive cells is 0 or at least the minimum, and refuses the bundle otherwise. | **done in sprint 1** |
| A3 | P1 | 9/10 | `estimate.py` site-type loop; evidence `{"sites": 1}` on every provincial parameter | The study has one provincial site, so every "provincial hospital" value is that one site's value. It reaches the sponsor report, which breaks PRD §5.9: "No site-identifiable output goes to a third party without that site's written consent." | Mark any site-type value from fewer than 3 sites as `single_site`. Withhold it from third-party reports unless the package records that site's consent, and pool it with other types otherwise. A test covers it. | **done in sprint 1** |
| A4 | P2 | 9/10 | `golden_path.py:21-25` imports both zones | P-4 decided on two processes. The code runs both zones in one process, with the boundary enforced only by an import test. | Add a `python -m hospital_zone` command that writes to `exchange/` and a `python -m platform_zone` command that reads it. `golden_path` drives both as subprocesses, and a test shows the platform process never opens a hospital path. | done in sprint 2 (`tests/test_two_process.py`) |
| A5 | P2 | 8/10 | CLAUDE.md "Stack": "A job queue for simulation runs" | F3.7 needs 1,000 runs re-run within an hour. Measured: 1,000 runs take under 0.1 seconds, and the whole path takes 2 seconds. A queue adds a service with nothing to do. | Remove the job queue from the demo stack, and revisit it at F2.7 (master protocols). | **done in sprint 1** |
| A6 | P1 before real data | 8/10 | `golden_path.py`: `hz / "pseudonym.key"` beside `clinical.db` | The hospital database is a plain SQLite file, and the pseudonym key sits next to it. demo-architecture.md requires encryption with hospital-held keys. | Use full-disk encryption on the hospital server, keep the key outside the data folder with owner-only permissions, and add a startup check that refuses to run on an unencrypted volume in non-synthetic mode. SQLite stays in the hospital zone; PostgreSQL stays the platform target. | pending |
| A7 | P1 for the schedule | 9/10 | `platform_zone/documents/extract.py:17` `_PASSAGE` | The extractor only reads lines shaped `[P7.1] text`. Real protocols have no such markers, so on a real document it returns nothing. It reads fixtures; it is not yet a baseline. | Epic 2 needs Word and PDF text extraction with page and paragraph anchors, then field extraction through the gateway, then human review of every field. Re-estimate Epic 2 with this in it; the sprint plan does. | pending |
| A8 | P2 | 6/10 | `estimate.py` `observed_treatment_difference` | The export carries the observed treatment effect. For a sponsored study that is a confidential result. Medium confidence, verify this is actually an issue: Life AI's own investigator-initiated studies may be fine. | Export it only when the package's data-use terms allow it; suppress it by default for sponsored studies. | pending |

## 2. Code quality

| # | Severity | Confidence | Where | Finding | Recommended remedy | State |
|---|---|---|---|---|---|---|
| C1 | P1 before real data | 9/10 | `hospital_zone/intake/deidentify.py:26-27` | De-identification works from a deny-list: four identifier columns and one free-text column. Any other free-text column (adverse-event text, withdrawal reason) passes with only a phone and ID-number check, so a name in it leaves the intake step. | Switch to an allow-list. Only columns mapped to the clinical schema pass; unmapped columns are dropped and logged; free text is excluded unless a local model is configured. A test puts a name in a reason column. | **done in sprint 1** |
| C2 | P2 | 9/10 | `estimate.py` and `platform_zone/simulator/engine.py` `SITE_TYPES`; report and page text | Site types are hard-coded as central and provincial. A district hospital or private clinic in a real study would be dropped silently from the estimates. | Derive site types from the sites table, and let the engine and report iterate over the types present. Test with three types. | pending |
| C3 | P2 | 8/10 | `extract.py`, `platform_zone/feedback/digest.py` | Vietnamese patterns and the lexicon are written in composed (NFC) form. Text pulled from PDFs, and some Word files, arrives decomposed (NFD), and then "Tiêu chuẩn" does not match. | Normalise to NFC at intake for documents and CSV files, with a test that feeds NFD input. | **done in sprint 1** |
| C4 | P3 | 9/10 | `calibrate.py`, `tests/test_replay.py` use `store._db` | Callers reach into a private connection. | Add `ClinicalStore.close()` and a test helper for mutation. | **done in sprint 1** |
| C5 | P3 | 8/10 | `platform_zone/profile/store.py` `_new_version` | Every review snapshots the whole profile: 20 reviews give 21 versions of 22 rows. Fine at demo scale, quadratic later. | Leave as is for the demo. Move to row versions when the profile store pools studies (F1.7). | pending |
| C6 | P1 for the team | 9/10 | `shared/version.py` hashes source files because there is no git | No version control and no CI, while BACKLOG 0.1 requires CI. The code version cannot name a commit. | Run `git init` and make a first commit. CI runs pytest, `calibrate --seeds 30` and the golden path; `code_version()` uses the commit hash once one exists. | **done in sprint 1** |

## 3. Tests

Framework: pytest, with 71 tests in `tests/`. Coverage of the built code:

```
CODE PATHS                                              USER FLOWS
[+] hospital_zone                                       [+] Golden path (one run, both zones)
  ├── deidentify       [★★★] identifiers, free text,      ├── [★★★ TESTED] end to end, all pages — test_golden_path.py
  │                     phone in kept column, keys         ├── [★★★ TESTED] re-run reproduces report hash
  │   └── [GAP] name in an unmapped free-text column (C1)  └── [GAP] [→E2E] two-process run, platform never
  ├── ClinicalStore    [★★★] consent, point-in-time view,          opens hospital paths (A4)
  │                     leakage rewrite test               [+] Clinician review
  ├── estimate         [★★★] hand computations, truth      ├── [★★ TESTED] confirm / correct / reject in store
  │   └── [GAP] three site types (C2)                      └── [GAP] [→E2E] review screen (not built)
  ├── replay/back-test [★★ ] leakage, interval shape       [+] Sign-off
  ├── export.gate      [★★★] small cell, field, pseudonym,  ├── [★★★ TESTED] unsigned / changed report blocked
  │                     tamper, role                       └── [GAP] [→E2E] sign-off screen (not built)
  │   ├── [GAP] differencing across cells (A2)
  │   ├── [GAP] signature verification (A1)            [+] Error states a user sees
  │   └── [GAP] single-site values withheld (A3)          ├── [★★ TESTED] export refusal reasons
[+] platform_zone                                          └── [GAP] refusal shown on screen (no web app yet)
  ├── documents        [★★ ] synthetic EN/VI, disagreement
  │   ├── [GAP] [→EVAL] real Word/PDF protocols vs hand-labelled gold set (A7)
  │   └── [GAP] NFD input (C3)
  ├── digest           [★  ] synthetic lexicon agreement (checks code, not method)
  │   └── [GAP] [→EVAL] real letters vs hand-labelled themes
  ├── profile store    [★★★] schema, label rule, review, tamper
  ├── simulator        [★★★] reproducibility, analytic power, type I error
  ├── report           [★★★] unlabelled number, unknown quantity, signer, VI rendering
  └── static pages     [★  ] banner on every page
      └── [GAP] every number on profile and simulate pages carries a label
calibrate              [★★ ] 30-seed bounds in the suite

COVERAGE: 17/30 paths tested (57%) | Code paths: 12/21 (57%) | User flows: 5/9 (56%)
QUALITY: ★★★ 10  ★★ 5  ★ 2 | GAPS: 13 (3 E2E, 2 eval)
```

**Critical regression requirement (pending the owner's answer on C1).** Switching de-identification to an allow-list can drop a column the estimator needs. The regression test runs the golden path before and after and asserts that the exported parameters are identical, including their hashes.

**Tests the remedies need.** Each is pending with its remedy:
- `tests/test_export_gate.py`: unsigned bundle refused, wrong key refused, small difference between cells refused.
- `tests/test_site_types.py`: three site types estimated; a single-site value withheld from the third-party report.
- `tests/test_documents.py`: NFD input normalised; a real-format protocol (Word and PDF fixture written by the team, no patient data) extracted with page anchors.
- `tests/test_two_process.py`: the golden path as two subprocesses.

## 4. Performance

No issues found. Measured on this machine:
- golden path: 2 seconds
- 1,000 simulated trials: under 0.1 seconds
- 10,000 null runs: under 1 second
- calibration on 150 studies: 45 seconds

The one performance consequence is A5: the job queue is not needed.

## Outside voice

Unavailable. The Codex command needs a git repository, and trial-os is not one; the native fallback needs task tools this session does not have. No second-model coverage, and this review gets no clean credit for it.

## Failure modes

| Path | Realistic failure | Handled? | User sees |
|---|---|---|---|
| Export transfer | A bundle edited in transit, with its hash re-computed | No (A1) | Nothing: the edited values are imported. **Critical gap.** |
| Export content | Monthly counts under 10 derivable by subtraction | No (A2) | Nothing. **Critical gap.** |
| Report to sponsor | Values that identify one site | No (A3) | Nothing. **Critical gap.** |
| De-identification | A name in a free-text column | No (C1) | Nothing until the estimator; the name never leaves the hospital zone, but it sits unprotected in the clinical store. |
| Real protocol | No passage markers, so no fields extracted | Partly: the fields are missing and the record build fails loudly | An error from `build_study_record` |
| Unicode | NFD text fails to match | No (C3) | Fields missing for review |
| Hospital disk | Server disk unencrypted | No (A6) | Nothing |

**Critical gaps: 3** (A1, A2, A3). None affects the synthetic demo. All three block real data.

## NOT in scope

- PostgreSQL in the hospital zone: SQLite on an encrypted disk is enough for one study.
- The job queue (A5).
- Electronic signatures to ICH E6(R3) (F9.9); signing here is a named sign-off with a hash.
- Pooled profiles (F1.7) and row-level profile versions (C5).

## What already exists

The golden path, the calibration check, the export gate, the profile store with label rule and review, the simulator checked against analytic power, and the report with its no-raw-numbers rule. None of it is rebuilt by the remedies above, which harden it in place.

## Worktree parallelization

| Step | Modules touched | Depends on |
|---|---|---|
| Git, CI (C6) | repo root, CI config | — |
| Export signing, difference gate, single-site rule (A1–A3) | `hospital_zone/estimator`, `shared/` | Git |
| Allow-list de-identification, NFC (C1, C3) | `hospital_zone/intake`, `platform_zone/documents` | Git |
| Site types generalised (C2) | `hospital_zone/estimator`, `platform_zone/simulator`, `platform_zone/reports` | A1–A3 (shared estimator) |
| Two processes (A4) | `golden_path.py`, new CLIs in each zone | A1 (signed bundle is the interface) |
| Real-document pipeline (A7) | `platform_zone/documents`, `platform_zone/gateway` | Git |

Lane A runs A1–A3 and then C2, sharing `hospital_zone/estimator`. Lane B runs C1 and C3. Lane C runs A7. Lane D runs A4 after A1. Launch A, B and C together after git; D starts when A1 merges. Conflict flag: `shared/` is touched by A1–A3 and C3; land A first.

## Implementation tasks

Synthesized from this review's findings. Each task derives from a specific finding above. All are pending owner confirmation.

- [ ] **T1 (P1, human: ~1h / CC: ~5min)** — repo — git init, first commit, CI with pytest, calibration (30 seeds) and golden path
  - Surfaced by: Code quality C6
  - Files: repo root, CI config, `shared/version.py`
  - Verify: CI green on the first push
- [ ] **T2 (P1, human: ~1d / CC: ~30min)** — export — sign bundle and approval with a hospital Ed25519 key; platform verifies
  - Surfaced by: Architecture A1
  - Files: `hospital_zone/estimator/export.py`, `platform_zone/profile/store.py`, `shared/export_format.py`, tests
  - Verify: `pytest tests/test_export_gate.py tests/test_profile_store.py`
- [ ] **T3 (P1, human: ~1d / CC: ~30min)** — export — quarterly series and a differencing check in the gate
  - Surfaced by: Architecture A2
  - Files: `hospital_zone/estimator/estimate.py`, `hospital_zone/estimator/export.py`, tests
  - Verify: the probe in this review finds 0 recoverable counts below 10
- [ ] **T4 (P1, human: ~1d / CC: ~30min)** — profile and report — single-site rule and per-site consent flag
  - Surfaced by: Architecture A3
  - Files: `estimate.py`, `platform_zone/profile/store.py`, `platform_zone/reports/compose.py`, tests
  - Verify: the sponsor report shows no single-site value without consent
- [ ] **T5 (P1, human: ~2d / CC: ~45min)** — intake — allow-list de-identification with a golden-path regression test
  - Surfaced by: Code quality C1
  - Files: `hospital_zone/intake/deidentify.py`, tests
  - Verify: a name in a free-text column never reaches the store; exported parameters unchanged
- [ ] **T6 (P1, human: ~3w / CC: ~1w with review)** — documents — Word and PDF text with anchors, gateway extraction, field review
  - Surfaced by: Architecture A7
  - Files: `platform_zone/documents/`, `platform_zone/gateway/`, `gold/`
  - Verify: field accuracy on a hand-labelled set of 200 fields (F1.2)
- [x] **T7 (P2, human: ~1d / CC: ~30min)** — zones — two CLIs and a two-process golden path
  - Surfaced by: Architecture A4
  - Files: `hospital_zone/__main__.py`, `platform_zone/__main__.py`, `golden_path.py`, tests
  - Verify: `pytest tests/test_two_process.py`
- [x] **T8 (P2, human: ~1d / CC: ~30min)** — estimator and simulator — site types from data
  - Surfaced by: Code quality C2
  - Files: `estimate.py`, `engine.py`, `compose.py`, `static_site.py`, tests
  - Verify: three-type synthetic study runs end to end
- [ ] **T9 (P2, human: ~2h / CC: ~10min)** — intake — NFC normalisation
  - Surfaced by: Code quality C3
  - Files: `platform_zone/documents/intake.py`, `extract.py`, `deidentify.py`, tests
  - Verify: NFD fixture extracts the same fields
- [ ] **T10 (P1 before real data, human: ~1d ops / CC: n/a)** — hospital server — disk encryption, key placement, startup check
  - Surfaced by: Architecture A6
  - Files: `hospital_zone/store/clinical_store.py` (startup check), server runbook
  - Verify: non-synthetic mode refuses an unencrypted volume
- [x] **T11 (P2, human: ~2h / CC: ~10min)** — export — treatment effect gated by data-use terms
  - Surfaced by: Architecture A8
  - Files: `estimate.py`, `export.py`, consent and terms metadata
  - Verify: sponsored-study bundle carries no observed effect
- [ ] **T12 (P3, human: ~1h / CC: ~5min)** — store — `close()` and test helper; drop the job queue from the stack
  - Surfaced by: C4, A5
  - Files: `clinical_store.py`, `calibrate.py`, `tests/`, `CLAUDE.md`
  - Verify: no `._db` outside the store

## Unresolved decisions that may bite you later

A1, A2, A3, A4, A5, A6, A7, A8, C1, C2, C3, C4, C5 and C6. Each has a recommended remedy above and no owner answer. A1, A2, A3, A6 and C1 must be answered before real data loads.

## Completion summary

- Step 0, Scope Challenge: scope accepted as built; complexity gate recorded, arrangement kept
- Architecture review: 8 issues found
- Code quality review: 6 issues found
- Test review: diagram produced, 13 gaps identified
- Performance review: 0 issues found
- NOT in scope: written
- What already exists: written
- TODOS.md updates: 0 (the project uses BACKLOG.md; tasks go to the sprint plan)
- Failure modes: 3 critical gaps flagged
- Unresolved decisions: 14 in this review
- Outside voice: Codex, unavailable (not a git repository; native fallback tools absent)
- Parallelization: 4 lanes, 3 parallel / 1 sequential
- Lake Score: N/A (no coverage choices answered)

## GSTACK REVIEW REPORT

| Review | Trigger | Why | Runs | Status | Findings |
|--------|---------|-----|------|--------|----------|
| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | DONE_WITH_CONCERNS (not logged) | 6 proposals, 6 accepted, 0 deferred |
| Outside Review | Codex via `/plan-eng-review` | Independent 2nd opinion | 1 | unavailable | none |
| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 1 | ISSUES OPEN | 27 issues, 3 critical gaps |
| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | — |
| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | — |

- **OUTSIDE COVERAGE:** Codex, plan-review phase, unavailable (not a git repository); no native fallback ran.
- **VERDICT:** No review CLEAR. Eng review has open issues: eng review required before real data; the synthetic demo build can continue.

**UNRESOLVED DECISIONS:**
- A4 two processes in code (sprint 2)
- A6 disk encryption and key placement (P1, before real data; sprint 3)
- A7 real-document pipeline scope in Epic 2 (sprint 2)
- A8 observed effect gated by data-use terms (sprint 2)
- C2 site types from data in the simulator and report (sprint 2; the estimator already reads them)
- C5 row-level profile versions later